gro: remove per-command --json (BREAKING) [#144] - #147
Conversation
JSON is reserved for round-trip payloads and control-plane envelopes per cli-common docs/output-and-rendering.md §2. Today's surviving JSON surfaces are 'gro refresh --json' (§4.6, added in #146) and 'gro config show --json' (diagnostic). Every resource-surface leaf under mail/, calendar/, contacts/, drive/, me/ now emits text only; running 'gro mail search foo --json' returns cobra's 'unknown flag: --json'. The boundary is uniform across resource reads AND mutation-success renderers — mail archive/label/markread/star/categorize/draft, calendar color/rsvp, contacts group_manage/star, drive star all lose their --json flag along with the read commands. Architecture: inverts golden principle #4 + the structural test that enforced it. Pre-#144: every leaf had to declare --json/-j (with two binary-write exemptions). Post-#144: every resource leaf must NOT declare --json — caught by TestResourceLeavesHaveNoJSONFlag. The jsonExemptCommands allowlist is gone (empty after the inversion). Helper cleanup: bulk.Result.Print(jsonOutput bool) → Print() (text only); the four package-local printJSON helpers in mail/calendar/contacts/drive output.go are deleted; me/output.go's RenderJSON + json-shaped structs are deleted. shared internal/output helpers stay (refresh + config show consume them). Tests: per-package 'has json flag' subtests, every Test*_JSON function, and noleak's me --json variants are removed; config show --json no-leak coverage stays. BREAKING CHANGE: 'gro <resource> --json' now errors. Scripts that piped 'gro ... --json | jq' must move to text parsing or use one of the surviving JSON surfaces (gro refresh --json, gro config show --json). Closes #144
|
Findings
Everything else I checked lines up with the intended boundary: no residual resource command Verification run: |
The pre-#144 instructions required output.go to contain a printJSON() helper and every leaf to declare --json/-j. Both are inverted by the new policy: resource-surface leaves emit text only; JSON is reserved for control-plane envelopes outside the domain resource packages (refresh, config show). Updated the section so a future domain author follows the §4 rule rather than the removed surface.
|
Findings
Verified targeted tests pass: |
Two doc tails referenced the removed printJSON() helper: - docs/architecture.md file-naming table - internal/output/output.go's JSON() docstring referring to 'per-domain printJSON() callers' (those are gone; the only surviving JSONStdout callers are control-plane refresh + config show).
TDD Coverage AssessmentCoverage of new behavior: The PR's core invariant (resource-surface leaves declare no Gaps:
Redundancy: None observed. The deleted Risk level: Low |
…-out Two TDD-suggested coverage adds: - TestResourceLeaf_RejectsJSON_EndToEnd dispatches 'gro drive drives --json' through cobra and asserts the user-visible 'unknown flag' error. The static guard catches missing flag declarations; this one catches a hypothetical regression where the flag is declared but the structural test's domain set is bypassed. - TestConfigShowCommand: assert the --json control-plane flag is declared with -j shorthand. config show --json had no-leak coverage in internal/noleak but no shape coverage; this pins the carve-out surface itself.
monit-reviewer
left a comment
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: 1f767c8
Summary
| Reviewer | Findings |
|---|---|
| harness-engineering:harness-enforcement-reviewer | 3 |
| harness-engineering:harness-knowledge-reviewer | 1 |
| harness-engineering:harness-self-documenting-code-reviewer | 1 |
harness-engineering:harness-enforcement-reviewer (3 findings)
💡 Suggestion - internal/output/output.go:19
Comment still references 'existing per-domain printJSON() callers', which were intentionally removed by this PR. Reword to reference the actual current callers (refresh --json, config show --json) or remove the sentence to avoid misleading future readers.
💡 Suggestion - internal/architecture/architecture_test.go:263
TestResourceLeaf_RejectsJSON_EndToEnd only tests 'drive drives' and its comment overstates the protection — if drive were dropped from domainCommands(), both tests would miss it simultaneously. Reword the comment to say 'spot-check complement to the structural walk' rather than implying it guards against domain-set bypass.
💡 Suggestion - docs/golden-principles.md:29
The new §4 text names the two carve-outs (refresh --json, config show --json) but does not articulate the criteria that qualify a command as a control-plane carve-out. One sentence on the qualifying criterion ('control-plane envelope or diagnostic introspection, outside domain resource packages') would prevent ad-hoc re-introduction by future contributors.
harness-engineering:harness-knowledge-reviewer (1 findings)
CLAUDE.md:40
The Key Constraints section still lists '--json on all leaf commands: Every leaf subcommand supports
--json/-j.' This directly contradicts the post-#144 inversion enforced by TestResourceLeavesHaveNoJSONFlag. AI agents and contributors using CLAUDE.md as guidance will attempt to re-add --json flags to new leaf commands, breaking the architecture test. Update to reflect that resource-surface leaves must NOT declare --json; JSON is reserved for control-plane envelopes (refresh --json, config show --json).
harness-engineering:harness-self-documenting-code-reviewer (1 findings)
💡 Suggestion - internal/cmd/drive/drives_test.go:27
Only drives_test.go and archive_test.go received the inverted 'no json flag' assertion form. All other per-command test files simply delete the old assertion without adding the inverted form. Either all domain command tests should carry the inverted assertion or none should — the current split makes per-command signal inconsistent, even though TestResourceLeavesHaveNoJSONFlag ensures global correctness.
6 info-level observations excluded. Run with --verbose to include.
3 PR discussion threads considered.
Completed in 3m 40s | $1.91 | sonnet | daemon 0.2.121 | Glorfindel
| Field | Value |
|---|---|
| Model | sonnet |
| Reviewers | hybrid-synthesis, documentation:docs-reviewer, harness-engineering:harness-architecture-reviewer, harness-engineering:harness-enforcement-reviewer, harness-engineering:harness-knowledge-reviewer, harness-engineering:harness-self-documenting-code-reviewer, security:security-code-auditor |
| Engine | claude · sonnet |
| Reviewed by | pr-review-daemon · monit-pr-reviewer |
| Duration | 3m 40s wall · 13m 00s compute (Reviewers: 3m 07s · Synthesis: 30s) |
| Cost | $1.91 |
| Tokens | 677.8k in / 48.2k out |
| Turns | 14 |
Per-workstream usage
| Workstream | Model | In | Out | Cache read | Cache create | Cost |
|---|---|---|---|---|---|---|
| hybrid-synthesis | sonnet | 32.4k | 1.9k | 0 | 32.4k (1h) | $0.16 |
| documentation:docs-reviewer | haiku | 11.6k | 4.9k | 0 | 11.6k (1h) | $0.05 |
| harness-engineering:harness-architecture-reviewer | sonnet | 207.1k | 10.1k | 158.6k | 48.5k (1h) | $0.42 |
| harness-engineering:harness-enforcement-reviewer | sonnet | 108.9k | 9.0k | 54.4k | 54.5k (1h) | $0.40 |
| harness-engineering:harness-knowledge-reviewer | sonnet | 50.4k | 7.7k | 4.0k | 46.3k (1h) | $0.33 |
| harness-engineering:harness-self-documenting-code-reviewer | sonnet | 216.5k | 8.8k | 162.4k | 54.1k (1h) | $0.42 |
| security:security-code-auditor | haiku | 50.9k | 5.7k | 0 | 50.9k (1h) | $0.13 |
Re-reviews only run when @monit-reviewer is re-requested as a reviewer — push as many commits as you need, then re-request when ready. PRs targeting branches other than main, master are skipped, even when @monit-reviewer is re-requested.
Note: Posted 4 inline comments. 1 finding remained in the summary because GitHub could not attach them to the current diff.
Five daemon findings on PR #147: 1. The repo's agent-guidance markdown §Key Constraints still required '--json on all leaf commands' — directly contradicts the post-#144 inversion and would mislead future contributors. Updated to mirror the new text-only rule. 2. internal/architecture E2E test comment overstated protection (claimed to catch a domain-set bypass when it shares the same list). Reworded as a spot-check complement that explicitly notes neither test guards against domainPackages drift. 3. docs/golden-principles.md §4 grew a 'carve-out criteria' paragraph articulating what qualifies a command as a control-plane carve-out (outside domain resource packages + envelope/diagnostic). 4. Removed the two duplicate per-command 'no json flag' subtests (drives_test.go, archive_test.go) for consistency — global guard in TestResourceLeavesHaveNoJSONFlag covers all leaves uniformly.
monit-reviewer
left a comment
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: 2a8772e | Previous: 1f767c8 (incremental)
Summary
No issues found.
7 PR discussion threads considered.
Completed in 1m 22s | $1.76 | sonnet | daemon 0.2.121 | Glorfindel
| Field | Value |
|---|---|
| Model | sonnet |
| Mode | Re-review · Cycle 2 · Session resumed |
| Reviewers | hybrid-synthesis, documentation:docs-reviewer, harness-engineering:harness-architecture-reviewer, harness-engineering:harness-enforcement-reviewer, harness-engineering:harness-knowledge-reviewer, harness-engineering:harness-self-documenting-code-reviewer, security:security-code-auditor |
| Engine | claude · sonnet |
| Reviewed by | pr-review-daemon · monit-pr-reviewer |
| Duration | 1m 22s wall · 1m 51s compute (Reviewers: 31s · Synthesis: 30s) |
| Cost | $1.76 |
| Tokens | 571.2k in / 8.2k out |
| Turns | 7 |
Per-workstream usage
| Workstream | Model | In | Out | Cache read | Cache create | Cost |
|---|---|---|---|---|---|---|
| hybrid-synthesis | sonnet | 39.5k | 1.6k | 13.2k | 26.2k (1h) | $0.13 |
| documentation:docs-reviewer | haiku | 24.1k | 4.5k | 0 | 24.1k (1h) | $0.05 |
| harness-engineering:harness-architecture-reviewer | sonnet | 99.1k | 103 | 4.0k | 95.0k (1h) | $0.36 |
| harness-engineering:harness-enforcement-reviewer | sonnet | 97.9k | 125 | 4.0k | 93.9k (1h) | $0.36 |
| harness-engineering:harness-knowledge-reviewer | sonnet | 96.5k | 82 | 4.0k | 92.4k (1h) | $0.35 |
| harness-engineering:harness-self-documenting-code-reviewer | sonnet | 97.7k | 144 | 4.0k | 93.7k (1h) | $0.35 |
| security:security-code-auditor | haiku | 95.1k | 1.2k | 0 | 95.1k (1h) | $0.12 |
| discussion-summarizer | — | 21.4k | 395 | 13.2k | 8.1k (1h) | $0.04 |
Re-reviews only run when @monit-reviewer is re-requested as a reviewer — push as many commits as you need, then re-request when ready. PRs targeting branches other than main, master are skipped, even when @monit-reviewer is re-requested.
|
No findings. The daemon edits look aligned with #144: stale docs/comments were cleaned up, Verified with targeted search and tests: |
Summary
Remove per-command `--json/-j` from every resource-surface leaf in gro (mail, calendar, contacts, drive, me) per cli-common `docs/output-and-rendering.md` §2 (JSON reserved for round-trip payloads + control-plane envelopes only). After this PR, attempting any `gro --json` returns cobra's `unknown flag: --json`.
Carve-outs (preserved unchanged):
`gro set-credential --json` does not exist today; if added later it would qualify as a §1.5.2 carve-out, but this PR neither adds nor presupposes that surface.
Boundary
"No per-command `--json` anywhere on resource surfaces, including mutation-success renderers." mail `archive/label/markread/star/categorize/draft`, calendar `color/rsvp`, contacts `group_manage/star`, drive `star` are swept up by the same policy as the resource reads — mirrors the cfl#392 boundary.
Architectural shift
Golden principle #4 is inverted:
`docs/golden-principles.md` §4 rewritten to match.
Shared helper cleanup
Test invariants
`TestResolveDriveScope` continues to fail on 2 subtests in some local environments — confirmed pre-existing on `main` via `git stash + test`, environment-dependent, unrelated to this PR.
Stats
74 files changed, 147 insertions(+), 1954 deletions(-). Net deletion: the removal absorbs more code than it adds.
Closes #144